Skip to content

Tests and simplification for ImuDeadReckoning#422

Open
brickbots wants to merge 4 commits into
mainfrom
idr_tests
Open

Tests and simplification for ImuDeadReckoning#422
brickbots wants to merge 4 commits into
mainfrom
idr_tests

Conversation

@brickbots
Copy link
Copy Markdown
Owner

This is an exploratory PR to see if several of the intermediate calculations can be dropped from the ImuDeadReckoning Class. Tests have been added to help validate the refactor and the previous implementation has been saved for side-by-side value validation.

The math involved here is not at all my strong point, so I relied on Claude for implementing my intuition that a few of these terms could drop out, especially if the existing Tetra3 camera pixel space offset alignment is relied on.

@TakKanekoGit this will really need your eyes to see if I've subtly broken something. The tests all look good and it seems to produce the same results. There may be further caching/optimization possible

Calculation Changes

Plate-solve update (solve)

  • Same end result for q_eq2x, written in a more compact form: q_eq2x = q_eq2pointing · (q_x2imu · q_imu2pointing).conj() instead of the old q_eq2cam · q_cam2imu · q_x2imu.conj(). Same value (using
    the identity q_cam2imu · q_x2imu.conj() = (q_x2imu · q_imu2cam).conj()), but it removes the separately stored q_cam2imu = q_imu2cam.conj() — the conjugate is taken inline at the one site that needs
    it.
  • q_eq2cam is no longer cached as a side effect — the radec→quaternion conversion still happens, but the result is consumed immediately to form q_eq2x and then discarded.
  • The "plate solve without IMU updates only the cam cache" branch is gone. Because there's no cache, a solve() with NaN q_x2imu is now a pure no-op rather than a partial update.

Dead-reckoning (predict)

  • Collapsed from two multiplies to one. Old: q_eq2cam = q_eq2x · q_x2imu · q_imu2cam, then q_eq2scope = q_eq2cam · q_cam2scope (effectively the four-term product q_eq2x · q_x2imu · q_imu2cam ·
    q_cam2scope). New: single three-term product q_eq2pointing = q_eq2x · q_x2imu · q_imu2pointing, with q_imu2pointing standing in for q_imu2cam.
  • The trailing · q_cam2scope factor is gone because alignment is no longer applied (see below).
  • Normalization happens once (on the single output) instead of twice (once on each of q_eq2cam and q_eq2scope).
  • Predictions are computed on demand from q_eq2x and the live q_x2imu rather than read from cached q_eq2cam / q_eq2scope fields.

Alignment calculation — removed entirely

  • No more q_eq2cam · q_eq2scope.conj() solve for q_cam2scope.
  • No more q_imu2cam · q_cam2scope pre-composition for the (never-used) q_imu2scope.
  • q_imu2pointing is now exactly the old q_imu2cam from the screen_direction table — hardware geometry only, with no alignment factor folded in.

Sentinel / NaN handling

  • reset() now writes a NaN quaternion to q_eq2x instead of Python None, so the subsequent np.isnan(q_eq2x) check at the top of predict() works (the old code raised TypeError after a reset).
  • The two NaN gates (pointing.valid and np.isnan(q_x2imu)) are now in one place at the top of solve(), instead of split across update_plate_solve_and_imu / update_imu.

Per-screen-direction q_imu2pointing table

  • Bit-identical to the old q_imu2cam table. The values for flat, flat3, left, right, straight, and as_bloom are unchanged; only the attribute name moved.

Verified by the 14 cross-check tests in test_imu_dead_reckoning_equivalence.py: under identity alignment, the new predict() matches the old get_scope_radec() to 1e-9 radians across all six screen
directions through mixed solve/predict sequences.

Code Changes

Class changes (pointing_model/imu_dead_reckoning.py, ~230 → ~110 lines):

  • Collapsed the 4 stored body rotations (q_imu2cam, q_cam2imu, q_cam2scope, q_imu2scope) into a single q_imu2pointing set at construction from screen_direction. The IMU-to-pointing rotation is
    pre-composed once; downstream math is one quaternion-triple multiply per update instead of two.
  • API is now four methods: solve(pointing, q_x2imu), predict(q_x2imu) -> RaDecRoll | None, is_initialized(), reset(). The cached q_eq2cam/q_eq2scope and the dead_reckoning/tracking flags are gone;
    predictions are computed on demand.
  • solve() returns early on invalid pointing or NaN IMU; reset() now correctly sets q_eq2x to a NaN quaternion (the original was assigning None, which broke the subsequent np.isnan() check).
  • Behavior change: camera/scope alignment is no longer applied inside this class. Relies on pixel offset alignment via Tetra3 and the offset RA/DEC/ROLL is provided downstream into the integrator

Integrator (integrator.py):

  • Wrappers slimmed to thin deg/rad converters around solve() and predict().
  • set_cam2scope_alignment helper deleted; imu_dead_reckoning.tracking replaced with is_initialized().
  • IMU dead-reckoning writes the predicted pointing to both solved["RA"/"Dec"/"Roll"] and solved["camera_center"][...] (identical values, since alignment is no longer applied).

Tests:

  • tests/test_imu_dead_reckoning.py (17 tests) pins the new API: solve/predict/reset semantics, round-trip identity, IMU-delta rotation, state transitions.
  • tests/test_imu_dead_reckoning_equivalence.py (14 tests) drives the new class and a preserved copy of the old class through identical input sequences and asserts the scope-pointing outputs agree to
    1e-9 across all six screen_direction values.

Temporary scaffolding (delete in follow-up PR after field validation):

  • pointing_model/imu_dead_reckoning_legacy.py — verbatim copy of the pre-refactor class, renamed ImuDeadReckoningLegacy. Marked temporary in its module docstring.
  • tests/test_imu_dead_reckoning_equivalence.py — the cross-check suite above.

Test plan

  • pytest -m unit --ignore=tests/website — 158 passed, 0 failed
  • pytest tests/test_imu_dead_reckoning.py tests/test_imu_dead_reckoning_equivalence.py -v — 31 passed
  • ruff check on changed files — clean
  • mypy on changed files — clean (modulo pre-existing unrelated library-stub warnings)
  • Smoke-run on real hardware: python3 -m PiFinder.main starts cleanly, plate-solves and IMU dead-reckons through scope motion
  • On-device: confirm tracking quality during sweep is comparable to pre-refactor; check chart and SkySafari pointing during dead-reckoning intervals
  • Verify on a unit with non-trivial alignment that the behavior change (no alignment correction in this class) is working

Copy link
Copy Markdown
Contributor

@TakKanekoGit TakKanekoGit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's remarkable how compact and simple the code set for imu_dead_reckoning.py it's been refactored. It's really nice.

One issue (and it needs to be checked by Claude) is that it does not seem to account for the camera-to-scope alignment and it writes the camera-centre coordinates to solved["RA"]andsolved["Dec"].

Otherwise it looks great!

Comment thread python/PiFinder/integrator.py Outdated
pointing = imu_dead_reckoning.predict(q_x2imu)
assert pointing is not None # guaranteed by the is_initialized() check above
ra_deg, dec_deg, roll_deg = pointing.get(deg=True)
solved["RA"], solved["Dec"], solved["Roll"] = ra_deg, dec_deg, roll_deg
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pointing estimated by imu_dead_reckoning is at the camera centre and it's no longer converted to the scope pointing. pointing is assigned here to both solved["camera_center"] and solved["RA"] etc.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, it's just using the single solution in both places now. I'm hoping to remove the camera_center data entirely and just have the single set of pointing data flowing through. I still need to double check where that's used (probably align chart) and make sure to fix that up.

# The IMU's unkonwn drifting reference frame X. This is solved for
# every time we have a simultaneous plate solve and IMU measurement.
q_eq2x: quaternion.quaternion = quaternion.quaternion(np.nan) # nan means not set
q_imu2pointing: quaternion.quaternion
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think pointing refers to the camera center but I think it's ambiguous because I first thought it meant the scope. May I propose putting it back to using the cam convention? So here q_imu2cam and so on?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... this is a good point. If we move the mapping between the camera pointing location and the telescope pointing location back into the solver using the pixel offset, then we are only carrying through the one RA/Dec/Roll out of the solver... but pointing is pretty ambiguous.

We could make it very explicit q_imu2platesolve?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry... I shouldn't try to do this in between my day job tasks 😅 You are totally right, the term in question is the physical orientation of the camera and IMU set by the screen direction. I'm renaming it back to q_imu2cam as it clearly confused me!

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good :-)

If it helps, I looked at tetra3 and it seems to use RA and Dec for camera/image centre and RA_target and Dec_target for the coordinates at the target_pixel.

Comment thread python/PiFinder/integrator.py Outdated
pointing = imu_dead_reckoning.predict(q_x2imu)
assert pointing is not None # guaranteed by the is_initialized() check above
ra_deg, dec_deg, roll_deg = pointing.get(deg=True)
solved["RA"], solved["Dec"], solved["Roll"] = ra_deg, dec_deg, roll_deg
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a comment that after the integrator, we don't need to set the roll for scope: solved["Roll"]. It's there for historical reasons but it's no longer needed. No action needed. Just a note.

Copy link
Copy Markdown
Owner Author

@brickbots brickbots May 18, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make sure I understand... we don't need this any longer as the orientation of the chart is no longer driven by it after your recent changes?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. That's correct. We just need to output RA/Dec and Az/Alt (but only when in Alt/Az mount mode) from the integrator.py

@brickbots
Copy link
Copy Markdown
Owner Author

Thanks @TakKanekoGit for the review, it's very appreciated!

For this refactor, I'm heading back to relying solely on using the pixel offset in Tetra3 to get the RA/DEC of the pixel selected during alignment and then feed this through as the RA/DEC/Roll that everything else uses. So it's sort of baked in early as part of the plate solving... but we do loose the two solutions (aligned and camera center), but it reduces the calculations needed and hopefully simplifies most of the code that now only needs to deal with a single pointing location.

I think that the camera center only ended up only being used for the alignment UI. I need to do a followup or addition to this PR to fully remove that distinction and make sure the align screen still works as intended, probably by temporarily zeroing out the pixel offset while the alignment chart is being drawn 🤔

Another thing I'm keen to do, and may do it as part of this PR, is to replace all of the untyped dictionaries involved in the solving and integrator with dataclasses to make the whole thing much more legible and robust. Right not it's not at all clear what all the key's in the dictionary does, which are required where, and it's just a bit of mess that I've created 😅

brickbots and others added 2 commits May 18, 2026 14:37
The body rotation set from screen_direction is the IMU-to-camera
hardware geometry; naming it for that physical relationship is clearer
than naming it for its role in the math. Docstring notes that the class
treats the camera frame as the pointing frame.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
camera_center duplicated the IMU-tracked camera FoV pointing that's
already in top-level RA/Dec/Roll after the ImuDeadReckoning refactor.
The chart in non-align mode now reads top-level RA/Dec/Roll directly;
align mode continues to use camera_solve (frozen at the last plate
solve). The integrator's solve() input switches to camera_solve, which
is value-equivalent at solve time and the right semantic source.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@TakKanekoGit
Copy link
Copy Markdown
Contributor

Hi @brickbots

For this refactor, I'm heading back to relying solely on using the pixel offset in Tetra3 to get the RA/DEC of the pixel selected during alignment and then feed this through as the RA/DEC/Roll that everything else uses. So it's sort of baked in early as part of the plate solving... but we do loose the two solutions (aligned and camera center), but it reduces the calculations needed and hopefully simplifies most of the code that now only needs to deal with a single pointing location.

Although it adds an extra layer, I think we need to use the camera/image centre as the reference for seamless dead-reckoning with the IMU. This is because tetra3 gives RA, Dec and Roll at the camera centre but only gives the RA, Dec at the target_pixel. We need RA, Dec, Roll to define the orientation fully.

@TakKanekoGit
Copy link
Copy Markdown
Contributor

TakKanekoGit commented May 19, 2026

I think that the camera center only ended up only being used for the alignment UI. I need to do a followup or addition to this PR to fully remove that distinction and make sure the align screen still works as intended, probably by temporarily zeroing out the pixel offset while the alignment chart is being drawn 🤔

I have a wish list item that's completely irrelevant to this PR but it would be great if the PiFinder could remember the previous alignment and continue using it. For testing, I often have to restart the PiFinder so it'll be great if I don't have to re-align. As a user, I've had the battery die and had to re-align after plugging in a Power Bank 😀.

@TakKanekoGit
Copy link
Copy Markdown
Contributor

Another thing I'm keen to do, and may do it as part of this PR, is to replace all of the untyped dictionaries involved in the solving and integrator with dataclasses to make the whole thing much more legible and robust. Right not it's not at all clear what all the key's in the dictionary does, which are required where, and it's just a bit of mess that I've created 😅

That'll be really nice. You might have already spotted it but if you're able to use the RaDecRoll class you might be able to get rid of or simplify the helper functions at the bottom of integrator.py which are mainly interfacing between the dictionary format and RaDecRoll class.

In the same file (coordinates.py), there are outlines for other coordinate classes like RaDec and AltAz.

Wouldn't it be better to do it in a different PR because that's quite a big change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants